Hi @peterxcli @ivandika3
Could you review this? Appreciate!

@YutaLin could you also help me to take a look if we need to clear the etag field and expected data gen field?

@YutaLin please re-request my review if think this is ready, thanks!

omKeyInfo.setExpectedDataGeneration(null);
omKeyInfo.setExpectedETag(null);

Hi @peterxcli
We need to set them null for now, otherwise, they'll be stored in KeyTable later, which we don't want.
Once we've completed #10181, setting them to null will no longer be necessary, as the codec will prevent them from being stored in the KeyTable.

@YutaLin Thanks for the patch, left some comments, the rest looks good.

By the way, could you help me reason about how we can return a 409 in the presence of concurrent requests? Specifically, if two or more independent clients send the exact same Complete Multipart Upload request at the same time, only one client should receive a successful response and the others should get a 409. That way, they know they must re-read the object, re-upload the parts if necessary, and then complete the MPU again using the latest ETag they observed.

ref: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html

If-Match
Uploads the object only if the ETag (entity tag) value provided during the WRITE operation matches the ETag of the object in S3. If the ETag values do not match, the operation returns a 412 Precondition Failed error.
If a conflicting operation occurs during the upload S3 returns a 409 ConditionalRequestConflict response. On a 409 failure you should fetch the object's ETag, re-initiate the multipart upload with CreateMultipartUpload, and re-upload each part.

Hi @peterxcli
Conditional Request conflict can't happen because MPU completion uses pessimistic lock, meaning there's no race window between validation and commit.

I will make comments in file.

Hi @peterxcli
We need to set them null for now, otherwise, they'll be stored in KeyTable later, which we don't want.
Once we've completed #10181, setting them to null will no longer be necessary, as the codec will prevent them from being stored in the KeyTable.

yeah, for the key commit request, your statement is true, but I'm asking if S3MultipartUploadCompleteRequest we touched need to do the same thing.

• ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java

  Lines 310 to 312 in bbf1687

  	omKeyInfo = omKeyInfo.toBuilder()
  	.setExpectedDataGeneration(null)
  	.setExpectedETag(null)

• ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java

  Lines 249 to 250 in bbf1687

  	omKeyInfo.setExpectedDataGeneration(null);
  	omKeyInfo.setExpectedETag(null);

Hi @peterxcli,
In S3MultipartUploadCompleteRequest, we don't have to do that because those fields are not set in OpenKeyTable during the mpu process. We validate conditional resource(keyArgs) against the existing Keyinfo from the KeyTable.

For put requests, however, the conditional resource comes from the OpenKeyTable. Since it may already contain conditional values when stored, we need to set them to null beforehand.

Hi @peterxcli Conditional Request conflict can't happen because MPU completion uses pessimistic lock, meaning there's no race window between validation and commit.

yeah, I know, but the spec looks like AWS S3 can detect such cases and return 409.
Let me think about it more. For now, let’s just merge the current patch.

Thanks @YutaLin for the patch!